Skip to content

feat(client): add reconnectionScheduler to StreamableHTTPClientTransport#1763

Open
felixweinberger wants to merge 6 commits intomainfrom
fweinberger/reconnection-scheduler
Open

feat(client): add reconnectionScheduler to StreamableHTTPClientTransport#1763
felixweinberger wants to merge 6 commits intomainfrom
fweinberger/reconnection-scheduler

Conversation

@felixweinberger
Copy link
Copy Markdown
Contributor

Adds a ReconnectionScheduler callback option to StreamableHTTPClientTransportOptions so non-persistent environments can override the default setTimeout-based SSE reconnection scheduling.

Motivation and Context

Fixes #1162. The current _scheduleReconnection uses setTimeout, which doesn't work well for:

  • Serverless/edge functions that terminate before the timer fires
  • Mobile apps that need platform-specific background scheduling (iOS Background Fetch, Android WorkManager)
  • Desktop apps handling sleep/wake cycles

Supersedes #1177 with one API addition: the scheduler may return a cancel function that is invoked on transport.close(), so pending custom-scheduled reconnections can be aborted the same way the built-in setTimeout is cleared. Thanks @CHOIJEWON for the original implementation.

How Has This Been Tested?

6 new tests in packages/client/test/client/streamableHttp.test.ts:

  • scheduler invoked with (reconnect, delay, attemptCount)
  • falls back to setTimeout when no scheduler provided
  • setTimeout not used when scheduler provided
  • returned cancel function called on close()
  • tolerates schedulers returning void (no cancel)
  • default setTimeout still cleared on close()

pnpm --filter @modelcontextprotocol/client test passes (317 tests).

Breaking Changes

None. New optional option, default behavior unchanged. Internal _reconnectionTimeout field renamed to _cancelReconnection (private, not part of public API).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

API:

type ReconnectionScheduler = (
    reconnect: () => void,
    delay: number,
    attemptCount: number
) => (() => void) | void;

The _reconnectionTimeout?: ReturnType<typeof setTimeout> field is replaced by _cancelReconnection?: () => void, unifying cleanup for both the default and custom scheduler paths.

Adds a ReconnectionScheduler callback option so non-persistent
environments can override the default setTimeout-based SSE reconnection
scheduling.

The scheduler receives (reconnect, delay, attemptCount) and may return
a cancel function that is invoked on transport.close(). This ensures
pending custom-scheduled reconnections can be aborted the same way the
built-in setTimeout is cleared.

Replaces the _reconnectionTimeout field with a unified _cancelReconnection
callback that works for both the default and custom scheduler paths.

Fixes #1162
Closes #1177

Co-authored-by: CHOIJEWON <alsrn6040@kakao.com>
@felixweinberger felixweinberger requested a review from a team as a code owner March 25, 2026 21:26
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 25, 2026

🦋 Changeset detected

Latest commit: 6e24e54

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@modelcontextprotocol/client Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Mar 25, 2026

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/client@1763

@modelcontextprotocol/server

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/server@1763

@modelcontextprotocol/express

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/express@1763

@modelcontextprotocol/hono

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/hono@1763

@modelcontextprotocol/node

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/node@1763

commit: 6e24e54

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — clean, minimal API addition with good test coverage and no change to default behavior.

Extended reasoning...

Overview

This PR adds a single optional reconnectionScheduler callback to StreamableHTTPClientTransportOptions, allowing non-persistent environments (serverless, mobile, desktop sleep/wake) to override the default setTimeout-based SSE reconnection. The implementation touches one source file (streamableHttp.ts) and its test file, plus a changeset. The internal _reconnectionTimeout field is replaced by a unified _cancelReconnection callback that works for both the default setTimeout path and custom schedulers.

Security risks

None. The change is a scheduling mechanism for reconnection — it does not touch auth, credentials, permissions, or data handling. The scheduler callback receives only a reconnect function, a delay number, and an attempt count.

Level of scrutiny

Low-to-moderate. This is a new optional configuration option with no change to default behavior. The API surface is small (ReconnectionScheduler type + one optional property). The refactoring from _reconnectionTimeout to _cancelReconnection is purely internal (private fields). The implementation correctly handles void returns from the scheduler, clears _cancelReconnection before invoking reconnect(), and the close() method properly calls and nullifies the cancel function.

Other factors

  • 6 new focused tests cover: scheduler invocation with correct arguments, setTimeout fallback, cancel-on-close, void-tolerant schedulers, and clearTimeout on close without a scheduler.
  • 2 existing tests were updated to reference the renamed private field (_cancelReconnection instead of _reconnectionTimeout).
  • No CODEOWNERS-specific paths are affected beyond the default wildcard.
  • The changeset correctly marks this as a minor version bump.
  • No outstanding reviewer comments or unaddressed feedback in the timeline.

@felixweinberger
Copy link
Copy Markdown
Contributor Author

Picked this up as something we discussed previously @mattzcarey

Copy link
Copy Markdown
Contributor

@bhosmer-ant bhosmer-ant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice feature — the API design is clean (passing the computed delay as a suggestion is a good call) and the _cancelReconnection abstraction is cleaner than the old timeout field. A few things to tighten up around the weaker cancellation contract that a user-supplied scheduler introduces:

Also: ReconnectionScheduler isn't re-exported from packages/client/src/index.ts — users can't import the type to annotate their scheduler implementation without reaching into the internal path.

- guard reconnect() against late firing after close via aborted-signal check
- wrap _cancelReconnection in try/finally so close() always aborts and fires onclose
- wrap recursive _scheduleReconnection in try/catch to route scheduler errors to onerror instead of unhandled rejection
- export ReconnectionScheduler type from package index
- add tests for late-firing reconnect and throwing cancel function
bhosmer-ant
bhosmer-ant previously approved these changes Mar 27, 2026
Copy link
Copy Markdown
Contributor

@bhosmer-ant bhosmer-ant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All four findings addressed — the abort-guard, try/finally in close(), try/catch around the recursive _scheduleReconnection, and the type export are all correct and minimal. Two new tests cover the edge cases (late-firing reconnect after close, cancel-throws-but-abort-still-fires). Bughunter ran clean (21 candidates, 0 confirmed). LGTM.

One pre-existing minor inconsistency (not introduced here, not blocking): the _scheduleReconnection call at ~line 438 (graceful-disconnect path) isn't directly wrapped like the one at 459 — but it's inside the outer try, so a throwing scheduler there gets caught at line 447 with a slightly misleading "SSE stream disconnected" message. Optional follow-up at most.

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Extended reasoning...

Overview

This PR adds an optional reconnectionScheduler callback to StreamableHTTPClientTransportOptions, allowing non-persistent environments (serverless, mobile, desktop sleep/wake) to override the default setTimeout-based SSE reconnection. The change touches one source file (streamableHttp.ts), a new companion examples file, the package index re-export, and comprehensive tests.

Security risks

None. The change is additive — a new optional configuration callback. No auth, crypto, or permissions code is modified. The scheduler receives only a reconnect callback and delay/attempt metadata; it cannot access internal transport state.

Level of scrutiny

Medium. This is a well-scoped, non-breaking feature addition to a transport layer. All three reviewer suggestions from bhosmer-ant (abort guard in reconnect closure, try/finally in close(), try/catch around scheduler in .catch handler) have been incorporated and their comments resolved. My previous nit about the examples convention has also been addressed with a proper streamableHttp.examples.ts companion file. 8 new tests cover the scheduler, fallback, cancel, void-return, late-fire, and cancel-throws scenarios.

Other factors

  • Default behavior is unchanged (falls back to setTimeout)
  • Internal field rename (_reconnectionTimeout_cancelReconnection) is private API only
  • Changeset included for minor version bump
  • Existing tests updated to use the new internal field name
  • No CODEOWNERS concerns for these files

bhosmer-ant
bhosmer-ant previously approved these changes Mar 27, 2026
Copy link
Copy Markdown
Contributor

@bhosmer-ant bhosmer-ant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving after main merge + the .examples.ts companion file move (per repo convention). No functional changes to the scheduler code since my previous approval.

Comment on lines +354 to +358
if (this._reconnectionScheduler) {
const cancel = this._reconnectionScheduler(reconnect, delay, attemptCount);
if (typeof cancel === 'function') {
this._cancelReconnection = cancel;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: When the custom reconnectionScheduler returns void (no cancel function), _cancelReconnection is not reset to undefined, so it can retain a stale cancel function from a prior _scheduleReconnection call. The setTimeout path (line 360) always sets _cancelReconnection, but the scheduler path only sets it conditionally. Fix: this._cancelReconnection = typeof cancel === "function" ? cancel : undefined;

Extended reasoning...

When _scheduleReconnection is called and a custom reconnectionScheduler is provided, the return value is checked with typeof cancel === "function". If it is a function, _cancelReconnection is set. However, if the scheduler returns void, _cancelReconnection is left untouched — it retains whatever value it held from a previous call.

Compare this with the default setTimeout path, which unconditionally sets _cancelReconnection = () => clearTimeout(handle). The two branches have inconsistent behavior: one always updates the field, the other only conditionally updates it.

Step-by-step proof: Consider two SSE streams (GET + POST) that disconnect concurrently:

  1. Stream A disconnects. _scheduleReconnection is called. The custom scheduler returns cancelA. Now _cancelReconnection = cancelA.
  2. Before cancelA fires, Stream B disconnects. _scheduleReconnection is called again. This time the scheduler returns void (e.g., it delegates to a fire-and-forget platform API). Since typeof undefined \!== "function", the if branch is skipped. _cancelReconnection still holds cancelA — a stale reference.
  3. On close(), this._cancelReconnection?.() calls the stale cancelA instead of having no cancel for Stream B's reconnection. Stream B's pending reconnection cannot be cancelled via this mechanism.

The practical impact is very low. The reconnect closure already guards against a closed transport with if (this._abortController?.signal.aborted) return (line 343), so a late-firing reconnect after close() is safely no-oped. Calling a stale cancel function is typically harmless (a no-op on an already-fired callback). The scenario also requires specific concurrent stream timing that is uncommon in practice.

The fix is a one-line change to unconditionally assign _cancelReconnection:

this._cancelReconnection = typeof cancel === "function" ? cancel : undefined;

This maintains the invariant that _cancelReconnection always reflects the currently scheduled reconnection, consistent with the setTimeout path.

Copy link
Copy Markdown
Contributor

@bhosmer-ant bhosmer-ant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the void-return edge case — when _scheduleReconnection is invoked from _handleSseStream (not via the reconnect closure which pre-clears), a void-returning scheduler would leave the previous cancel stale. The always-set ternary is the right fix. LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow customizable reconnection behavior for non-persistent clients

2 participants